Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 53.95%. Comparing base (86dd529) to head (008ef9d).
⚠️ Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main      #40      +/-   ##
==========================================
- Coverage   54.08%   53.95%   -0.13%     
==========================================
  Files          41       41              
  Lines        5985     6016      +31     
  Branches      807      810       +3     
==========================================
+ Hits         3237     3246       +9     
- Misses       2399     2420      +21     
- Partials      349      350       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Thanks, I'll have to think about this, since this will likely slow down simulations with analytical potentials a lot. Note that I think we already flush FMS.out which can be checked that the simulation is progressing. I do agree that it might be useful to print stuff more often in a more typical case of ab initio simulations.

Side note, I am currently on holidays for the next two weeks so will be unlikely to do much reviewing.

Thanks for the feedback!

Thanks, I'll have to think about this, since this will likely slow down simulations with analytical potentials a lot. Note that I think we already flush FMS.out which can be checked that the simulation is progressing.

That makes sense, and you're right that FMS.out lets you track the progress of the dynamics.

I do agree that it might be useful to print stuff more often in a more typical case of ab initio simulations.

Would an opt-in flag (e.g., flushAllOutputs=.true.) work? Happy to implement it if you think it's worthwhile. For context: on difficult trajectories I've tailed these outputs to verify restart correctness and check sensitivity to tightened thresholds (e.g., reproducibility of Amp and N values). This is especially helpful for CC dynamics, where 10 time steps can be an hour of wall time and early termination can save a lot of time.

Apologies for slow response here, as I wasn't sure what's the best course of action here.

Would an opt-in flag (e.g., flushAllOutputs=.true.) work? Happy to implement it if you think it's worthwhile.

I think having flushAllOutputs flag would be useful, but I honestly don't want to have if (flushAllOutputs) peppered all over the place. I think we'd need to have a centralized file-handling module, where we could then iterate over all open files and flush them.

Given the overall state of the codebase, I don't feel like such a refactor is super-high priority at the moment, but I understand that it might be more important for others.
@eirik-kjonstad if this is high priority for you, than I am happy to be persuaded, perhaps for some partial solution (e.g. just flushing the most important files)?

Apologies for slow response here, as I wasn't sure what's the best course of action here.

Would an opt-in flag (e.g., flushAllOutputs=.true.) work? Happy to implement it if you think it's worthwhile.

I think having flushAllOutputs flag would be useful, but I honestly don't want to have if (flushAllOutputs) peppered all over the place. I think we'd need to have a centralized file-handling module, where we could then iterate over all open files and flush them.

Given the overall state of the codebase, I don't feel like such a refactor is super-high priority at the moment, but I understand that it might be more important for others.

Thanks for the feedback. That makes sense. Totally understand that this is lower down on the list of things to prioritize.

@eirik-kjonstad if this is high priority for you, than I am happy to be persuaded, perhaps for some partial solution (e.g. just flushing the most important files)?

I think flushing all the files is not especially high-priority to me. The current version of our interface will log any request made by FMS, so there are ways for us to know what is going on (without having to tail FMS output files).

However, with additional testing I've noticed that also FMS.out is not flushed consistently. (In one example I did not see any Time: ... print until after a successful spawn at 220 au.) My preference here would be to at least have FMS.out consistently flush (possibly with an opt-in flag) at every timestep and decision point (e.g., rejection or entering spawning mode). Happy to implement this specifically – and leave the other files as-is – if you think it is a good idea (I already have the required flushes in a branch).

Perhaps a more compelling reason to flush a subset of outputs is that we often run "open-ended" dynamics simulations where we don't know how far we will get with our computational resources. In these cases it would be unfortunate if significant amounts of time-data has already been generated by OpenFMS but there's no way for us to extract it. In my experience, I've found it most useful to have N.dat, Amp.*, and positions.* , as these let us see the populations, geometries, and make it possible to simulate time-resolved spectroscopies. So these files could be candidates for a restricted subset of flushed files.

In my experience, I've found it most useful to have N.dat, Amp., and positions.

Yep, happy to flush these unconditionally for now, feel free to open a new PR or modify this one.

However, with additional testing I've noticed that also FMS.out is not flushed consistently. (In one example I did not see any Time: ... print until after a successful spawn at 220 au.) My preference here would be to at least have FMS.out consistently flush (possibly with an opt-in flag) at every timestep and decision point (e.g., rejection or entering spawning mode).

Yeah, interesting. My initial instinct would be to flush this file at the end of every time step, but the spawning algorithm and the adaptive time step make that more complicated. Feel free to add flushes at some critical points as well.

Opened a separate PR to flush some files unconditionally, #51. Closing this one.